Refactor task module: rename protocol.py to requests.py and reduce redundancy#2667
Refactor task module: rename protocol.py to requests.py and reduce redundancy#2667
Conversation
…dundancy Renamed protocol.py to requests.py for clarity - it handles MCP task request endpoints (tasks/get, tasks/result, tasks/cancel, tasks/list). Consolidated constants to config.py as single source of truth: - DEFAULT_POLL_INTERVAL_MS now derived from DEFAULT_POLL_INTERVAL - TaskConfig uses constant instead of hardcoded timedelta(seconds=5) Extracted _lookup_task_execution() helper to eliminate ~50 lines of duplicated Redis lookup code. Uses redis.mget() for single round-trip instead of 3 separate calls (performance improvement).
marvin-context-protocol
Bot
added
the
enhancement
WalkthroughThis pull request refactors task handler configuration and imports within the task server module. It introduces three new module-level constants in the config file—DEFAULT_POLL_INTERVAL, DEFAULT_POLL_INTERVAL_MS, and DEFAULT_TTL_MS—to centralize task defaults. A new internal helper function Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/fastmcp/server/tasks/requests.py (3)
42-46: Consider adding type hints fordocketand return type.The
docketparameter and the first element of the return tuple are typed asAny. While this may be necessary to avoid circular imports, consider using string literals for forward references or importing underTYPE_CHECKINGfor better type safety.🔎 Suggested improvement
from typing import TYPE_CHECKING if TYPE_CHECKING: from docket import Docket from docket.execution import Execution async def _lookup_task_execution( docket: "Docket", session_id: str, client_task_id: str, ) -> tuple["Execution", str | None, int]:
213-235: Consider using_lookup_task_executionhelper here as well.This handler still performs its own Redis lookup for task_key and execution, duplicating logic that exists in
_lookup_task_execution. While the handler doesn't needcreated_atorpoll_interval_ms, you could still use the helper and discard the unused values, or extract a smaller helper for just the task_key → execution lookup.This is a minor consistency suggestion that could be addressed in a follow-up.
403-405: PotentialValueErrorifcreated_atis malformed.
datetime.fromisoformat()can raiseValueErrorifcreated_atcontains an invalid ISO format string. While the value is stored by your own code and should be well-formed, consider defensive handling for robustness.🔎 Suggested fix
- createdAt=datetime.fromisoformat(created_at) - if created_at - else datetime.now(timezone.utc), + createdAt=( + datetime.fromisoformat(created_at) + if created_at + else datetime.now(timezone.utc) + ) + if not created_at or _is_valid_isoformat(created_at) + else datetime.now(timezone.utc),Or more simply, wrap in try/except:
try: created_at_dt = datetime.fromisoformat(created_at) if created_at else datetime.now(timezone.utc) except ValueError: created_at_dt = datetime.now(timezone.utc)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/fastmcp/server/server.py(1 hunks)src/fastmcp/server/tasks/config.py(2 hunks)src/fastmcp/server/tasks/requests.py(8 hunks)src/fastmcp/server/tasks/subscriptions.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Python ≥ 3.10 with full type annotations
Never use bareexcept- be specific with exception types
Files:
src/fastmcp/server/tasks/config.pysrc/fastmcp/server/tasks/requests.pysrc/fastmcp/server/tasks/subscriptions.pysrc/fastmcp/server/server.py
🧠 Learnings (2)
📚 Learning: 2025-12-21T21:37:55.015Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.015Z
Learning: Applies to src/fastmcp/__init__.py : All module exports should be intentional - only re-export to `fastmcp.*` for fundamental types like `FastMCP` and `Client`, prefer users importing from specific submodules for specialized features
Applied to files:
src/fastmcp/server/tasks/subscriptions.pysrc/fastmcp/server/server.py
📚 Learning: 2025-12-21T21:37:55.015Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T21:37:55.015Z
Learning: Applies to src/fastmcp/**/__init__.py : Core types that define a module's purpose should be exported (e.g., `Middleware` from `fastmcp.server.middleware`), while specialized features can live in submodules
Applied to files:
src/fastmcp/server/server.py
🧬 Code graph analysis (1)
src/fastmcp/server/tasks/requests.py (3)
src/fastmcp/server/server.py (1)
docket(382-387)src/fastmcp/server/context.py (1)
session_id(360-409)src/fastmcp/client/tasks.py (1)
status(171-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
🔇 Additional comments (8)
src/fastmcp/server/tasks/subscriptions.py (1)
16-16: LGTM!The import path update correctly reflects the module rename from
protocol.pytorequests.py. TheDOCKET_TO_MCP_STATEsymbol is now sourced from the consolidatedrequestsmodule.src/fastmcp/server/server.py (1)
609-614: LGTM!The import path update correctly reflects the module rename from
protocol.pytorequests.py. The handler registration logic remains unchanged.src/fastmcp/server/tasks/config.py (2)
18-21: Good refactor: Centralized task defaults.The new constants consolidate task metadata defaults in a single location, improving maintainability. Deriving
DEFAULT_POLL_INTERVAL_MSfromDEFAULT_POLL_INTERVALensures consistency between the two representations.
55-55: LGTM!Using the constant
DEFAULT_POLL_INTERVALinstead of an inlinetimedelta(seconds=5)ensures the default is defined in one place.src/fastmcp/server/tasks/requests.py (4)
1-5: LGTM!The updated docstring accurately describes the module's purpose and clarifies that these handlers query/manage existing tasks.
68-72: Good optimization: Batch Redis fetch.Using
redis.mget()to fetch all three keys in a single round-trip is a good performance improvement over separateget()calls.
139-142: LGTM!The handler now uses the centralized
_lookup_task_executionhelper, reducing code duplication and ensuring consistent lookup logic.
388-395: LGTM on the helper usage and cancellation logic.The handler now uses the centralized
_lookup_task_executionhelper, and correctly usesexecution.keyfor the cancellation call. This aligns with the PR objective of reducing redundant Redis lookup logic.
1 participant
Improves clarity and reduces duplication in the task module by renaming
protocol.py→requests.pyand consolidating Redis lookup logic.Changes
Renamed for clarity:
src/fastmcp/server/tasks/protocol.py→requests.pyConsolidated constants:
DEFAULT_POLL_INTERVAL_MSandDEFAULT_TTL_MStoconfig.pyDEFAULT_POLL_INTERVAL_MSis now derived fromDEFAULT_POLL_INTERVALinstead of duplicatedTaskConfigto reference the constantReduced redundancy:
_lookup_task_execution()helper functionredis.mget()for single round-trip instead of 3 separate getsAll 198 task tests pass.